Fix dataset pickling #7
Conversation
Computing a dataset using "processes" or a Client scheduler currently fails due to a pickling error. This adds a corresponding test that will need to be fixed.
`PosixPath.open()` returns a `BufferedReader` that is not pickleable, breaking dask computation with `Client` or "processes" schedulers. This still uses `resources` to identify data file paths, but opens directly from the paths instead of file buffers to fix pickling.
There was a problem hiding this comment.
@aazuspan, I think I understand this issue and the fix that you've implemented, namely that the path is pickleable, whereas the file handle is not.
Again, I'll yield to your expertise on just this simple "pickleable" test versus the full-blown compute tests. Can you say a bit more about what other types of incompatibilities you might expect with other schedulers? If you did go the full compute route, could you mark the tests as slow and skip them in CI? The issue that you referenced looks pretty thorny, though.
I wouldn't say I expect other issues, but that's what I would've said before this issue too 😜
Yeah, I think this is a good idea. I'm leaning towards punting on this for now. We'll need to figure out a good way to test deferred Dask results once we get predictions up and running, so I'd suggest we revisit dataset tests once that system's in place. |
|
Good with me! |
|
Thanks for giving it a look! |
There was a bug in
_load_rasters_to_datasetthat causedcomputeto fail with any Dask scheduler that requires pickling (anything but threads) because the file buffer used in raster loading isn't pickleable. The easy fix is just to load the file path built byresourcesrather than the file buffer.For testing, I'm just directly checking that the dataset is pickleable. A more thorough test would be to fully compute the dataset with each scheduler to avoid any other surprise incompatibilities, but that would lead to some very slow tests as well as some complexity around setup/teardown with a
LocalCluster(see dask/distributed#3540).@grovduck let me know what you think about the testing here, and whether we should just bite the bullet and add in
computetests.